Skip to content

API for request MCUboot mode by the application #44512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

nvlsianpu
Copy link
Collaborator

@nvlsianpu nvlsianpu commented Apr 4, 2022

This API is added in order to make possible entering the DFU recovery mode by MCUboot if application has requested that.

PR which enable possibility for MCUBoot recovey on the application demnd

Placed all dependent properties in one if statement.

Signed-off-by: Andrzej Puzdrowski <[email protected]>
@github-actions github-actions bot added the area: API Changes to public APIs label Apr 4, 2022
@@ -30,6 +30,20 @@ extern void z_arm_nmi_init(void);
#define LOG_LEVEL CONFIG_SOC_LOG_LEVEL
LOG_MODULE_REGISTER(soc);

int soc_boot_mode_set(uint8_t mode)
{
nrf_power_gpregret2_set(NRF_POWER, mode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing return?

Introduced the API which allows the Application to request a
specific bootloader mode and the MCUboot to obtain it.
MCUboot might enter this mode once system reboot.
Implementations of this will use SoC specific resources,
like a reset retention register.

Signed-off-by: Andrzej Puzdrowski <[email protected]>
@nvlsianpu nvlsianpu force-pushed the boot/boot_mode_set branch from d7d19ac to e76f2a3 Compare April 5, 2022 16:44
@nvlsianpu nvlsianpu marked this pull request as ready for review April 5, 2022 16:49
@nvlsianpu nvlsianpu force-pushed the boot/boot_mode_set branch from e76f2a3 to 3c39517 Compare April 6, 2022 15:03
@nvlsianpu nvlsianpu requested a review from hakonfam April 6, 2022 15:12
@nvlsianpu nvlsianpu force-pushed the boot/boot_mode_set branch from 3c39517 to 1c3e9df Compare April 6, 2022 16:00
Implemented SoC specific function needed for posting the bootloader
mode by the application and fetching it by the MCUboot.

Signed-off-by: Andrzej Puzdrowski <[email protected]>API for request MCUboot mode
@mbolivar-nordic
Copy link
Contributor

@plskeggs FYI

Comment on lines +7 to +10
#ifndef ZEPHYR_INCLUDE_MCUBOOT_BOOT_MODE_H_

#define BOOT_MODE_REQ_NORMAL 0x00
#define BOOT_MODE_REQ_RECOVERY_DFU 0x01
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I've looked at zephyr's MCUboot related APIs, but I'm confused about this mix of "MCUBOOT" and "BOOT" in the namespace. Is this meant to be MCUboot-specific, or generic across all bootloaders?

If it is meant to be mcuboot-specific, is there any harm in using MCUBOOT consistently? (I think this is the real question I am trying to ask.)

If it is meant to be generic, why is MCUBOOT appearing sometimes here?

@@ -47,6 +47,22 @@ extern void z_arm_nmi_init(void);
#define LOG_LEVEL CONFIG_SOC_LOG_LEVEL
LOG_MODULE_REGISTER(soc);

int soc_mcuboot_mode_set(uint8_t mode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing wrong with this line of code, but the Signed-off-by: line in the commit message has the wrong format.

@@ -47,6 +47,22 @@ extern void z_arm_nmi_init(void);
#define LOG_LEVEL CONFIG_SOC_LOG_LEVEL
LOG_MODULE_REGISTER(soc);

int soc_mcuboot_mode_set(uint8_t mode)
{
nrf_power_gpregret2_set(NRF_POWER, mode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From spot-checking a product specification, the GPREGRET and GPREGRET2 registers seem to be available for application developer use too. Is there any risk in using them for this purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPREGRET is used by NCS internally for instance, it is also place where nRF SoC stores reboot-type in zephyr.

Copy link
Collaborator Author

@nvlsianpu nvlsianpu May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can use GPREGRET for transfer both the reboot-type and the recovery-dfu request. This implies that I need to introduce an API for retrieve reboot-type as well.
What your opinion @mbolivar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand your last comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you see usage of GPREGRET2 as the waste of resources I can make a better solution:

The idea would be to start using GPREGRET for request of recovery-dfu as well (along with reboot-type to keep of which it is used currently).
As intermediate steep I would implement API for retrieve reboot-type as there is sys_arch_reboot(int type) call but there is no a counterpart to it which would allows to check what a reboot-type was used.
Of course this mean that zephyr common API for get reboot type will be needed:
there is void sys_reboot(int type) but zephyr doesn't have API for retrieve the type

@mbolivar-nordic What is your opinion on that ^^ idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK, I think I understand now. No, if we're already using GPREGRET, from my side I see no harm in using GPREGRET2 as well. You're saying you're not aware of any customers already using this that will be negatively impacted, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternative: #46084

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 27, 2022
@nvlsianpu nvlsianpu removed the Stale label Jul 27, 2022
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@nvlsianpu
Copy link
Collaborator Author

superseded by #50864

@nvlsianpu nvlsianpu closed this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants